-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix transaction run reporting #2617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes on this file are unrelated to the PR's main objective, but they can help us in the future.
We are changing all calls to os.Exit
here to a cliExit
function to help us to mock os.Exit
calls (which we use to debug CLI e2e tests).
@@ -31,7 +31,7 @@ var testRunCmd = &cobra.Command{ | |||
environmentOptions := append(baseOptions, actions.WithClient(utils.GetResourceAPIClient("environments", cliConfig))) | |||
environmentActions := actions.NewEnvironmentsActions(environmentOptions...) | |||
|
|||
runTestAction := actions.NewRunTestAction(cliConfig, cliLogger, client, environmentActions) | |||
runTestAction := actions.NewRunTestAction(cliConfig, cliLogger, client, environmentActions, ExitCLI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related with this comment: https://github.com/kubeshop/tracetest/pull/2617/files#r1209657040
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! the loom was a great explanation of the issue and fix!
This PR fixes the transaction run reporting that sometimes can be shown incorrectly to the user on both FE and CLI.
Context: https://www.loom.com/share/581dbc852ef140bc9dc150977cc08c5b
Test against OTel demo: https://www.loom.com/share/58ca611ad27c47248cd4221fca5770f4
Changes
Fixes
Checklist